Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ascent: SoA Particle Support #3350

Merged
merged 7 commits into from
Nov 21, 2023
Merged

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Jun 6, 2023

Summary

Add support for pure SoA layouted particle containers for Ascent.

Additional background

Follow-up to #2878.

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

@ax3l
Copy link
Member Author

ax3l commented Aug 10, 2023

@atmyers @c-wetterer-nelson @cyrush @andrewcombs let's coordinate how we finalize this - this is the last element blocking the AMReX SoA transition for WarpX on our agenda 🥁

@c-wetterer-nelson
Copy link
Contributor

@atmyers @c-wetterer-nelson @cyrush let's coordinate how we finalize this - this is the last element blocking the AMReX SoA transition for WarpX on our agenda 🥁

RE: the catalyst 2 support PR. Yesterday was Andrew's last day so I will take over. The last piece there is a GitHub action script to add catalyst build testing to the CI. We could do that in a separate PR if you're comfortable with that. I am hoping to find time next week to make that happen. Otherwise, we are ready to review that PR.

@@ -32,7 +31,7 @@ ParticleTileToBlueprint(const ParticleTile<amrex::Particle<NStructReal,
const std::string &topology_name)
{
int num_particles = ptile.GetArrayOfStructs().size();
int struct_size = sizeof(Particle<NStructReal, NStructInt>);
int struct_size = ParticleType::is_soa_particle ? 0 : sizeof(ParticleType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

struct_size is used to represent element striding in the Conduit tree.

For the SOA case - I expect we want native striding (for example, 8 bytes for float64), so I think we need to update the Particle Container to Conduit wrappers to handle these cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Cyrus - where is that wrapper located? I thought it was this file...

#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the style of logic we need.

If AOS, we stride by the struct size. If SOA, we stride by the native stride (set_external w/o the stride argument accomplishes this)

// struct real fields, the first set are always the particle positions
// which we wrap above
for (int i = 0; i < NStructReal; i++)
if constexpr(!ParticleType::is_soa_particle)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to build the fields for the SOA case, not sure how to access from AMReX but it should be a an else case.

@ax3l ax3l changed the title [WIP] Ascent: SoA Particle Support Ascent: SoA Particle Support Oct 4, 2023
@ax3l
Copy link
Member Author

ax3l commented Nov 20, 2023

Restarting CI.

From local tests, I think CI does not catch that his PR still has a compile error: #3639

@ax3l ax3l closed this Nov 20, 2023
@ax3l ax3l reopened this Nov 20, 2023
@ax3l ax3l mentioned this pull request Nov 20, 2023
5 tasks
@ax3l
Copy link
Member Author

ax3l commented Nov 20, 2023

Based on .github/workflows/ascent.yml and #3639, testing locally with:

docker run -v $PWD:/opt/amrex -it alpinedav/ascent:0.9.2
export CXX=g++
export CC=gcc
export CMAKE_PREFIX_PATH=/ascent/install/lib/cmake/

. /ascent_docker_setup_env.sh

cd /opt/amrex/
cmake -S . -B build          \
           -DCMAKE_BUILD_TYPE=RelWithDebInfo \
           -DAMReX_ENABLE_TESTS=ON  \
           -DAMReX_FORTRAN=OFF      \
           -DAMReX_ASCENT=ON   \
           -DAMReX_CONDUIT=ON  \
           -DAMReX_PARTICLES=ON
cmake --build build -j 8

WeiqunZhang pushed a commit that referenced this pull request Nov 20, 2023
## Summary

For some reason, `AMReX_ASCENT` does not trigger `AMReX_CONDUIT` to be
set, too.

cc @cyrush 

## Additional background

X-ref: #3350
@ax3l
Copy link
Member Author

ax3l commented Nov 21, 2023

I think our main.cpp is not yet complete :)

@atmyers atmyers merged commit 9e35dc1 into AMReX-Codes:development Nov 21, 2023
69 checks passed
@ax3l ax3l deleted the topic-soa-ascent branch November 27, 2023 18:00
guj pushed a commit to guj/amrex that referenced this pull request Dec 13, 2023
## Summary

For some reason, `AMReX_ASCENT` does not trigger `AMReX_CONDUIT` to be
set, too.

cc @cyrush 

## Additional background

X-ref: AMReX-Codes#3350
guj pushed a commit to guj/amrex that referenced this pull request Dec 13, 2023
## Summary

Add support for pure SoA layouted particle containers for Ascent.

## Additional background

Follow-up to AMReX-Codes#2878.

## Checklist

The proposed changes:
- [ ] fix a bug or incorrect behavior in AMReX
- [x] add new capabilities to AMReX
- [ ] changes answers in the test suite to more than roundoff level
- [ ] are likely to significantly affect the results of downstream AMReX
users
- [ ] include documentation in the code and/or rst files, if appropriate

---------

Co-authored-by: Andrew Myers <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants